Fix VB Conversions #31486

hughbe · 2018-07-30T17:25:32Z

Fixes https://github.com/dotnet/corefx/issues/31482

danmoseley · 2018-07-30T22:47:11Z

src/Microsoft.VisualBasic/src/Microsoft/VisualBasic/CompilerServices/Conversions.vb

+
+        End Function
+
+        <CLSCompliant(False)>


You can remove any CLSCompliant attributes. We don't use them.

Hmm, I get various errors if I do this though:

"C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\Microsoft.VisualBasic.sln" (default target) (1) -> "C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\tests\Microsoft.VisualBasic.Tests.csproj.metaproj" (d efault target) (2) -> "C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj.metaproj" (default t arget) (3) -> "C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj" (default target) (5 ) -> (CoreCompile target) -> C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(322,32): error BC40027: Return type of function 'ToSByte' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub\ corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(343,32): error BC40027: Return type of function 'ToSByte' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub\ corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(613,32): error BC40027: Return type of function 'ToUShort' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub \corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(634,32): error BC40027: Return type of function 'ToUShort' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub \corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(899,32): error BC40027: Return type of function 'ToUInteger' is not CLS-compliant. [C:\Users\hughb\Documents\GitH ub\corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(920,32): error BC40027: Return type of function 'ToUInteger' is not CLS-compliant. [C:\Users\hughb\Documents\GitH ub\corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(1189,32): error BC40027: Return type of function 'ToULong' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub \corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(1212,32): error BC40027: Return type of function 'ToULong' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub \corefx\src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(2201,55): error BC40028: Type of parameter 'Value' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub\corefx\ src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] C:\Users\hughb\Documents\GitHub\corefx\src\Microsoft.VisualBasic\src\Microsoft\VisualBasic\CompilerServices\Conversio ns.vb(2209,55): error BC40028: Type of parameter 'Value' is not CLS-compliant. [C:\Users\hughb\Documents\GitHub\corefx\ src\Microsoft.VisualBasic\src\Microsoft.VisualBasic.vbproj] 0 Warning(s) 10 Error(s)

Hmm OK fair enough

danmoseley · 2018-07-30T22:48:32Z

src/Microsoft.VisualBasic/src/Microsoft/VisualBasic/CompilerServices/Conversions.vb

+                    NumberStyles.AllowTrailingWhite Or
+                    NumberStyles.AllowCurrencySymbol
+
+


Nit, there are various double newlines in this file

danmoseley · 2018-07-30T23:02:01Z

src/Microsoft.VisualBasic/src/Microsoft/VisualBasic/CompilerServices/Conversions.vb

-            Return Value.ToString()
+
+        Public Shared Function FromCharArraySubset(ByVal Value() As Char, ByVal StartIndex As Integer, ByVal Length As Integer) As String
+            ' This is a private function used from the debug windows (VS7 264234)


Please remove references to old bug databases eg (VS7 264234) .. VS7 = Visual Studio 2002 ... I doubt any of us could find that bug database any more.

danmoseley · 2018-07-30T23:06:29Z

src/Microsoft.VisualBasic/tests/ConversionsTests.cs

+        {
+            Assert.Throws<InvalidOperationException>(() => Conversions.ToChar(value));
+        }
+
        private static object s_floatEnum;

        public static object FloatEnum


You can't make a float enum in VB or C#. Is it worth bothering to test this scenario? Ref.Emit is nice to avoid.

Same for bool and char. I would just skip them.

These edge cases used to throw a different exception in .NET Core compared to .NET Framework or threw when netfx didn't, so I would advocate keeping them. No harm really and there are already tests dotted around the place that use IntPtr/float etc. enums

karelz · 2018-09-05T17:43:35Z

@danmosemsft is there anything left from your side?
@cston @333fred @jaredpar can you please review and merge?

cston · 2018-09-05T19:46:16Z

src/Microsoft.VisualBasic/tests/ConversionsTests.cs

-            yield return new object[] { char.MinValue };
-            yield return new object[] { (char)1 };
-            yield return new object[] { char.MaxValue };
-            yield return new object[] { new DateTime(10) };


Are these cases no longer relevant?

I think they’re consolidated with the test data for other methods as they’re common with all other conversion methods

danmoseley · 2018-09-05T20:00:33Z

Do you want to rebase @hughbe then we can probably merge?

Fail with .NET Core

ViktorHofer · 2018-09-25T10:41:16Z

@dotnet-bot test this please

karelz · 2018-10-02T00:06:51Z

@cston @333fred can you please merge it?

333fred · 2018-10-02T00:28:51Z

@hughbe when I merge this I would like to squash the commits, do you have a problem if I do that on my end? Or would you prefer to manually squash them yourself?

karelz · 2018-10-02T00:46:10Z

@333fred please squash it, that's what we do, unless there is strong reason to keep the PR change history.

danmoseley · 2018-10-02T00:56:20Z

+1, in CoreFX and CoreCLR we always "squash and merge" unless there's good reason - I know other repos have different policies.

cston · 2018-10-02T18:12:58Z

Thanks @hughbe, this has been merged.

danmoseley reviewed Jul 30, 2018

View reviewed changes

joshfree added the area-Microsoft.VisualBasic label Jul 30, 2018

joshfree assigned OmarTawfik Jul 30, 2018

karelz assigned hughbe and jaredpar and unassigned OmarTawfik Aug 15, 2018

karelz assigned cston and 333fred Aug 23, 2018

cston reviewed Sep 5, 2018

View reviewed changes

cston approved these changes Sep 5, 2018

View reviewed changes

danmoseley approved these changes Sep 5, 2018

View reviewed changes

karelz added this to the 3.0 milestone Sep 6, 2018

hughbe added 3 commits September 8, 2018 22:23

Add Conversions tests that pass on netfx

ca2acd2

Fail with .NET Core

Replace corefx Conversions with reference source

2c4bb51

Address PR feedback and cleanup

90d3c44

cston merged commit 87ca813 into dotnet:master Oct 2, 2018

hughbe deleted the vb-conversion-fixes branch October 2, 2018 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VB Conversions #31486

Fix VB Conversions #31486

hughbe commented Jul 30, 2018

danmoseley Jul 30, 2018

hughbe Jul 30, 2018

danmoseley Jul 31, 2018

danmoseley Jul 30, 2018

danmoseley Jul 30, 2018

danmoseley Jul 30, 2018

danmoseley Jul 30, 2018

hughbe Jul 30, 2018

danmoseley Jul 31, 2018

karelz commented Sep 5, 2018

cston Sep 5, 2018

hughbe Sep 5, 2018

danmoseley commented Sep 5, 2018

ViktorHofer commented Sep 25, 2018

karelz commented Oct 2, 2018

333fred commented Oct 2, 2018

karelz commented Oct 2, 2018

danmoseley commented Oct 2, 2018

cston commented Oct 2, 2018

		NumberStyles.AllowTrailingWhite Or
		NumberStyles.AllowCurrencySymbol

Fix VB Conversions #31486

Fix VB Conversions #31486

Conversation

hughbe commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelz commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Sep 5, 2018

ViktorHofer commented Sep 25, 2018

karelz commented Oct 2, 2018

333fred commented Oct 2, 2018

karelz commented Oct 2, 2018

danmoseley commented Oct 2, 2018

cston commented Oct 2, 2018